Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add allowed ip check #605

Merged
merged 8 commits into from
Dec 14, 2021
Merged

Add allowed ip check #605

merged 8 commits into from
Dec 14, 2021

Conversation

marc-gr
Copy link
Contributor

@marc-gr marc-gr commented Dec 2, 2021

To identify any IP in the testing value that is not in the supported Maxmind's allowed set (found here https://github.com/elastic/elastic-package/blob/master/docs/howto/ingest_geoip.md) a validation check is added at test time.

Will output something similar to

0] parsing field value failed: the IP "107.170.139.210" is not one of the allowed test IPs
[1] parsing field value failed: the IP "96.241.146.97" is not one of the allowed test IPs

@marc-gr marc-gr requested a review from mtojek December 2, 2021 11:11
@elasticmachine
Copy link
Collaborator

elasticmachine commented Dec 2, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-12-14T12:04:02.293+0000

  • Duration: 56 min 33 sec

  • Commit: 610ebaa

Test stats 🧪

Test Results
Failed 0
Passed 605
Skipped 0
Total 605

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@mtojek mtojek requested a review from a team December 2, 2021 11:52
Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like a good PR, but I'm concerned about system tests. See the test output:

one or more errors found in documents stored in metrics-aws.ec2_metrics-ep data stream: [0] parsing field value failed: the IP "3.84.55.9" is not one of the allowed test IPs

internal/fields/validate.go Outdated Show resolved Hide resolved
@marc-gr marc-gr requested a review from mtojek December 2, 2021 14:55
Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only one left thing is adjusting pipeline tests?

internal/fields/validate.go Show resolved Hide resolved
@mtojek
Copy link
Contributor

mtojek commented Dec 3, 2021

I think you have to update pipeline tests for the AWS test package.

@marc-gr
Copy link
Contributor Author

marc-gr commented Dec 13, 2021

/test

@mtojek mtojek self-requested a review December 14, 2021 12:17
Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@marc-gr marc-gr merged commit 228a493 into elastic:master Dec 14, 2021
@marc-gr marc-gr deleted the allowed-ip-check branch December 14, 2021 13:51
func initializeAllowedIPsList() map[string]struct{} {
m := map[string]struct{}{
"0.0.0.0": {}, "255.255.255.255": {},
"0:0:0:0:0:0:0:0": {}, "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff": {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this testing takes output from the net package, this should also probably have "::" since an IPv6 address renders as the abbreviation.

}

if v.enabledAllowedIPCheck && !v.isAllowedIPValue(valStr) {
return fmt.Errorf("the IP %q is not one of the allowed test IPs", valStr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if this emitted a link to the list of acceptable IPs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants